Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve readability of brew upgrade #6436

Merged
merged 1 commit into from
Sep 13, 2019
Merged

Conversation

kluen
Copy link
Contributor

@kluen kluen commented Sep 9, 2019

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew tests with your changes locally?

Hi, this is just a small cosmetic change that I find useful.

When upgrading lots of packages at the same time, the list of packages can be hard to read due to the length of the list and the fact that most terminals by default do their line breaks exactly when the line is full and not on word endings. This often leads to chopped up package names and versions. It is not easy to quickly find a package at a glance.

For example, on my machine:
before
Note the line endings and beginnings.

With this change every package is printed on a new line. Package names are sorted alphabetically:
after

I'm assuming that the output is for human interaction only and will not be parsed by other tools?

Also, Is the order of the packages important? They are sorted alphabetically now to make packages easy to find at a glance. Should we rather display them in the order they will be upgraded?

Not sure if this change is wanted at all, but I'd be happy for ideas for improvements.

@MikeMcQuaid
Copy link
Member

Personally I prefer the prior output even although I agree it's not ideal with extremely large numbers of packages. @Homebrew/maintainers: thoughts?

@SMillerDev
Copy link
Member

I think the latter might be nicer. I don't have a strong opinion about either one.

@fxcoudert
Copy link
Member

Sorted output is what many other package managers (conda, yum) do. I find it a lot more readable.

@vitorgalvao
Copy link
Member

I prefer the latter (one per line), though I find the order of upgrading more important than alphabetical — in case of failure failure or requiring an interruption, it’s easier to know what was already done.

@idoo
Copy link
Contributor

idoo commented Sep 9, 2019

Could we add some flag that allows us to format it as described above or even make it silently?

@reitermarkus
Copy link
Member

I'd prefer sorted, and ", " if it fits in one line, otherwise "\n".

@sjackman
Copy link
Member

sjackman commented Sep 9, 2019

I prefer sorted by name (or possibly order of installation), and a mild preference for one per line.

@vszakats
Copy link
Contributor

vszakats commented Sep 10, 2019

As the Updated Formulae section is already sorted alphabetically, I'd prefer keeping the Upgrading N outdated packages: section in order of upgrading.

I'd keep the current format (which is compact and fairly readable when updating often), if it doesn't exceed a few lines (say 3), and switch to line-by-line beyond that.

@sjackman
Copy link
Member

sjackman commented Sep 10, 2019

I feel that switching the formatting conditionally is not worth the effort, and I'd prefer a consistent output format.

@kluen
Copy link
Contributor Author

kluen commented Sep 10, 2019

@sjackman I don't think effort is a problem here (looks like it would just be a one-liner). However, I do agree with you on the consistent format.

@zbeekman
Copy link
Contributor

I think the proposed changes (1 package per line) are easier to read.

A flag or environment variable to sort alphabetically or topo-sorted by dependency/update order would be nice, but if we have to pick only one, then I would say keep existing sorting on update-order/dependency topo-sort as it's trivial to pipe the output into the sort command to get alphabetic sorting.

The only drawback I see is that if someone doesn't set their terminal scrollback history/buffer to be long enough they may lose the beginning of the output if there is an absurdly large number of packages to upgrade...

@@ -98,14 +98,14 @@ def upgrade
else
verb = args.dry_run? ? "Would upgrade" : "Upgrading"
oh1 "#{verb} #{formulae_to_install.count} outdated #{"package".pluralize(formulae_to_install.count)}:"
formulae_upgrades = formulae_to_install.map do |f|
formulae_upgrades = formulae_to_install.sort.map do |f|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove the .sort here? Will keep the \n below though. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done :)

@kluen
Copy link
Contributor Author

kluen commented Sep 13, 2019

@MikeMcQuaid Just realized i forgot to squash the commits. It is rebased now.

@MikeMcQuaid MikeMcQuaid merged commit 9ba8c32 into Homebrew:master Sep 13, 2019
@MikeMcQuaid
Copy link
Member

Perfect!
Thanks so much for your contribution! Without people like you submitting PRs we couldn't run this project. You rock @kluen!,

@lock lock bot added the outdated PR was locked due to age label Jan 1, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jan 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants